Allows Vulkan spec constants as attribute arguments. by danbrown-amd · Pull Request #7439 · microsoft/DirectXShaderCompiler

danbrown-amd · 2025-05-08T03:01:24Z

github-actions · 2025-05-08T03:02:18Z

✅ With the latest revision this PR passed the C/C++ code formatter.

s-perron

I don't mind this change generally. It will be good for SPIR-V.

I don't know if this is complete yet (it is still a draft). I pointed out one issue with using LocalSize. You might also need to look at SpirvEmitter::addDerivativeGroupExecutionMode(). That function determines which derivative group execution mode to add based on the number of threas in the group. However, if you have a spec constant for the number of threads, we cannot do that.

danbrown-amd · 2025-06-17T04:59:44Z

Why was SpirvEmitter::addDerivativeGroupExecutionMode() written that way? It seems to me that it wouldn't be that difficult to provide enough context to find the original attribute in the AST so the method wouldn't need to scan the previously emitted instructions.

Of course, since there's no access to specializations at compile time, there's no way to ensure the best choice of execution mode with spec constants anyway. However, in some cases partial information would be sufficient to rule out one of them. If both are possible, maybe pick one and issue a warning?

s-perron · 2025-07-07T13:23:06Z

Why was SpirvEmitter::addDerivativeGroupExecutionMode() written that way?

It was implemented that way because that was the HLSL spec implemented for DXIL. See Thread Groups and Quads. I don't mind if we diverge from it when there are specialization constants. We just need to document it.

If both are possible, maybe pick one and issue a warning?

That is a reasonable idea. Longer term, we could try to add explicit attribute that apply to the entry point that would override the guess.

s-perron

LGTM, but requires some tests.

llvm-beanz

I think there is a lot of missing test coverage here. You're making a bunch of attributes that had previously either required integer literal arguments or optional integer literal arguments now support constant expressions and specialization constants.

We really need test coverage for each attribute's change in behavior, and the error states. In particular it doesn't seem to me like you're actually handling the possible error of a non-constant expression being provided as a value. I think it just silently replaces the value with the default (which would be bad).

llvm-beanz · 2025-07-21T20:52:15Z

 } // namespace

+static uint32_t GetIntConstAttrArg(ASTContext &astContext, const Expr *expr,
+                                   uint32_t defaultVal = 0) {


Suggested change

uint32_t defaultVal = 0) {

uint32_t defaultVal) {

Doesn't look like this is ever called without a default value.

I've changed this to use llvm::Optional so that an absent default indicates that expr should be non-null. (See note on SemaHLSL below.)

llvm-beanz · 2025-07-21T20:55:11Z

+const uint NumThreadsY = 1;
+#endif
+
+[numthreads(NumThreadsX,NumThreadsY,1)]


Some other test cases that are probably interesting:

uint CBufferValue; [numthreads(CbufferValue, 1, 1)] // Should error void main() {}

const uint Eight = 8; [numthreads(Eight * 2, Eight, 1)] // Should work void main() {}

The latter won't work but would if declared static. Larger expressions incorporating spec constants don't work in the current version.

I've added preprocessor directives to vk.spec-constants.attributes.hlsl to enable removal of the spec constant designations to test for expected errors.

llvm-beanz · 2025-07-21T21:00:33Z

+static uint32_t GetIntConstAttrArg(ASTContext &astContext, const Expr *expr,
+                                   uint32_t defaultVal = 0) {


Suggested change

static uint32_t GetIntConstAttrArg(ASTContext &astContext, const Expr *expr,

uint32_t defaultVal = 0) {

static uint32_t GetIntConstAttrArg(ASTContext &astContext, const Expr *expr,

uint32_t defaultVal) {

It is really unfortunate to completely duplicate this code, and I don't think the two should be identical. In this version in Sema we should be surfacing an error if there is an expression that isn't a constant expression or spec constant.

Sema already seems to complain in all the relevant cases, though I have added some more diagnostics. Of the arguments allowed to be spec constants, the only optional one is the NodeId index, so maybe just return zero (possibly after issuing an error) whenever expr is null? For now the function works as described for the CGHLSLMS version.

llvm-beanz · 2025-07-21T21:01:50Z

+      return (uint32_t)apsInt.getSExtValue();
+    if (expr->isVulkanSpecConstantExpr(astContext, &apValue) && apValue.isInt())
+      return (uint32_t)apValue.getInt().getSExtValue();
+  }


Suggested change

}

llvm_unreachable("Expression must be a constant expression or spec constant")

}

We should be erroring in Sema if this isn't a constant expression or spec constant.

Changed as suggested.

llvm-beanz · 2025-07-21T21:04:15Z

 };
 } // namespace

+static uint32_t GetIntConstAttrArg(ASTContext &astContext, const Expr *expr,


You've got a fair amount of mixing coding style, and DXC is a bit inconsistent.

In general if you're adding code in a part of the codebase that isn't otherwise following a consistent style we follow the LLVM Coding Standards. LLVM uses CamelCase for variable names and type names (see: https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly).

s-perron · 2025-11-05T15:00:34Z

/AzurePipelines run

azure-pipelines · 2025-11-05T15:00:48Z

Azure Pipelines successfully started running 1 pipeline(s).

…tributes). Fixes microsoft#3092.

github-project-automation Bot added this to HLSL Roadmap May 8, 2025

github-project-automation Bot moved this to New in HLSL Roadmap May 8, 2025

danbrown-amd force-pushed the attr-arg-consts branch 3 times, most recently from 9db3b6c to 243acbc Compare May 13, 2025 22:08

s-perron reviewed May 14, 2025

View reviewed changes

Comment thread tools/clang/lib/AST/ExprConstant.cpp

s-perron reviewed May 14, 2025

View reviewed changes

Comment thread tools/clang/lib/SPIRV/SpirvEmitter.cpp

s-perron reviewed May 14, 2025

View reviewed changes

danbrown-amd mentioned this pull request May 21, 2025

[spirv] AMD work graphs extension #7353

Merged

danbrown-amd force-pushed the attr-arg-consts branch 3 times, most recently from 357813d to 8e898bf Compare June 16, 2025 22:05

danbrown-amd marked this pull request as ready for review June 19, 2025 18:27

danbrown-amd changed the title ~~Allows defined constants as attribute arguments.~~ Allows Vulkan spec constants as attribute arguments. Jun 26, 2025

s-perron reviewed Jul 7, 2025

View reviewed changes

Comment thread tools/clang/lib/SPIRV/SpirvEmitter.cpp

Comment thread tools/clang/lib/SPIRV/SpirvEmitter.cpp Outdated

Comment thread tools/clang/lib/SPIRV/SpirvEmitter.cpp

s-perron mentioned this pull request Jul 8, 2025

[SPIR-V] Allow thread group size to be specified with specialization constants #3092

Open

s-perron approved these changes Jul 11, 2025

View reviewed changes

Comment thread tools/clang/test/CodeGenSPIRV/vk.spec-constant.attributes.derivative-group.hlsl

Comment thread tools/clang/test/CodeGenSPIRV/vk.spec-constant.attributes.derivative-group.hlsl

s-perron requested a review from llvm-beanz July 11, 2025 17:48

llvm-beanz reviewed Jul 21, 2025

View reviewed changes

damyanp assigned llvm-beanz Oct 20, 2025

[spirv] Allows spec constants as attribute arguments (for selected at…

21fdad3

…tributes). Fixes microsoft#3092.

danbrown-amd force-pushed the attr-arg-consts branch from 58b3508 to 21fdad3 Compare November 6, 2025 18:51

		static uint32_t GetIntConstAttrArg(ASTContext &astContext, const Expr *expr,
		uint32_t defaultVal = 0) {

	}
	llvm_unreachable("Expression must be a constant expression or spec constant")
	}

Conversation

danbrown-amd commented May 8, 2025 • edited by s-perron Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

s-perron left a comment

Choose a reason for hiding this comment

Uh oh!

danbrown-amd commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-perron commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-perron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

llvm-beanz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-perron commented Nov 5, 2025

Uh oh!

azure-pipelines Bot commented Nov 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

danbrown-amd commented May 8, 2025 •

edited by s-perron

Loading

github-actions Bot commented May 8, 2025 •

edited

Loading

danbrown-amd commented Jun 17, 2025 •

edited

Loading

s-perron commented Jul 7, 2025 •

edited

Loading